Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added basic yamux connection upgrade #1096

Merged
merged 1 commit into from
Dec 10, 2019
Merged

Added basic yamux connection upgrade #1096

merged 1 commit into from
Dec 10, 2019

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Dec 4, 2019

Description

Yamux provides multiplexing over a ordered reliable connection (e.g. TCP).
More info: https://github.com/hashicorp/yamux/blob/master/spec.md

  • Added yamux as a git submodule. Run git submodule init and git submodule update --recursive --remote. Optionally, run git config submodule.recurse true to enable updating all submodules on pull.
  • This is working from a fork of yamux (develop branch), where the
    upgrade to futures 0.3.x is close to complete
  • Made a TcpStream wrapper struct which implements futures AsyncWrite and AsyncRead to reduce the tie in to tokio
  • Side note: investigated upgrading to futures 0.3.x however there ended up being a few external libraries which are locked to alpha futures. This is WIP for tonic (Upgrade to tokio 0.2 hyperium/tonic#163) and tower (no PR - we may be able to pretty easily remove this dependency)

Motivation and Context

Partial #1049

How Has This Been Tested?

Unit tests

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Feature refactor (No new feature or functional changes, but performance or technical debt improvements)
  • New Tests
  • Documentation

Checklist:

  • I'm merging against the development branch
  • I ran cargo-fmt --all before pushing
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@sdbondi sdbondi force-pushed the sb-comms-yamux branch 4 times, most recently from 1a25279 to e418555 Compare December 4, 2019 06:21
comms/Cargo.toml Outdated Show resolved Hide resolved
@sdbondi sdbondi force-pushed the sb-comms-yamux branch 8 times, most recently from f5f490a to 030391f Compare December 9, 2019 15:26
@CjS77
Copy link
Collaborator

CjS77 commented Dec 9, 2019

Looks good; but would you mind adding your HOWTO to the README file?

People won't know to run git init submodules ... and will moan when cargo build fails

@sdbondi sdbondi force-pushed the sb-comms-yamux branch 5 times, most recently from 0415cf2 to f8b5574 Compare December 10, 2019 06:37
@@ -35,6 +35,20 @@ to generate the documentation. The generated html sits in `target/doc/`. Alterna

See [RFC-0110/CodeStructure](./RFC/src/RFC-0010_CodeStructure.md) for details on the code structure and layout.

### Git submodules
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CjS77 Added this section

Yamux provides multiplexing over a ordered reliable connection (e.g. TCP).
More info: https://github.com/hashicorp/yamux/blob/master/spec.md

- This is working from a fork of yamux (develop branch), where the
  upgrade to futures 0.3.x is close to complete
- Made a TcpStream wrapper struct which implements futures `AsyncWrite` and `AsyncRead` to reduce the tie in to tokio
- _Side note:_ investigated upgrading to futures 0.3.x however there ended up being a few external libraries which are locked to alpha futures. This is WIP for tonic (hyperium/tonic#163) and tower (no PR - we may be able to pretty easily remove this dependency)
Copy link
Collaborator

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

CjS77 added a commit that referenced this pull request Dec 10, 2019
Merge pull request #1096

Yamux provides multiplexing over a ordered reliable connection (e.g.
TCP).  More info: https://github.com/hashicorp/yamux/blob/master/spec.md

Added yamux as a git submodule. Run git submodule init and git submodule
update --recursive --remote. Optionally, run git config
submodule.recurse true to enable updating all submodules on pull.  This
is working from a fork of yamux (develop branch), where the upgrade to
futures 0.3.x is close to complete Made a TcpStream wrapper struct which
implements futures AsyncWrite and AsyncRead to reduce the tie in to
tokio Side note: investigated upgrading to futures 0.3.x however there
ended up being a few external libraries which are locked to alpha
futures. This is WIP for tonic (hyperium/tonic#163) and tower (no PR -
we may be able to pretty easily remove this dependency)
@CjS77 CjS77 merged commit 3c9774e into development Dec 10, 2019
@CjS77 CjS77 deleted the sb-comms-yamux branch December 10, 2019 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants